-
Notifications
You must be signed in to change notification settings - Fork 197
Update Elasticsearch exporter documentation #10586
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Clarified configuration options for sending queue and batcher settings in the Elasticsearch exporter documentation. Closes #10584
This pull request does not have a backport label. Could you fix it @inge4pres? 🙏
|
docs/reference/edot-collector/components/elasticsearchexporter.md
Outdated
Show resolved
Hide resolved
Pinging @elastic/ingest-docs (Team:Docs) |
stack: 9.0 9.1 | ||
``` | ||
|
||
The sending queue can be enabled and configured with the `batcher` section, using [common `batcher` settings](https://github.com/open-telemetry/opentelemetry-collector/blob/main/exporter/exporterhelper/internal/queue_sender.go). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not technically correct. In previous versions, sending_queue
and batcher
are both supported, but they are separate configs configuring the queue and batch correspondingly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok makes sense thanks - I thought sending_queue was a replacement for batcher, but no it's another setting on top.
How would you rephrase it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this paragraph (and the header) should focus on batcher
and state that to keep the interaction async after enabling batcher, one should enable sending queue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks I only have one doubt: sending_queue
does not seem to be aviable before v0.132.0.
if that's correct, we should not include a sending_queue section for 9.0 and 9.1.
I was thinking to structure the document with separate sections about sending queue and batching, but only if sending queue is available in 9.0 and 9.1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nvm I just re-read the docs in the upstream repo, sending_queue
was supported in older versions too.
I'll update with dedicated sections
docs/reference/edot-collector/components/elasticsearchexporter.md
Outdated
Show resolved
Hide resolved
docs/reference/edot-collector/components/elasticsearchexporter.md
Outdated
Show resolved
Hide resolved
Co-authored-by: Carson Ip <[email protected]>
docs/reference/edot-collector/components/elasticsearchexporter.md
Outdated
Show resolved
Hide resolved
stack: 9.0 9.1 | ||
``` | ||
|
||
The sending queue can be enabled and configured with the `batcher` section, using [common `batcher` settings](https://github.com/open-telemetry/opentelemetry-collector/blob/main/exporter/exporterhelper/internal/queue_sender.go). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this paragraph (and the header) should focus on batcher
and state that to keep the interaction async after enabling batcher, one should enable sending queue.
stack: 9.2 | ||
``` | ||
The Elasticsearch exporter supports the `sending_queue` setting, which supports both queueing and batching. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The first sentence kind of implies that sending queue was not supported in the past, which is not true. The key change in 9.2 (i'm not sure off the top of my head) is that we also support sending_queue::batch
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, makes more sens after reading the previous comments too.
Let me rephrase it.
I wonder: if this distinction is not so clear from the upstream repo docs, should we also amend there?
Co-authored-by: Fabrizio Ferri-Benedetti <[email protected]>
🔍 Preview links for changed docs |
Signed-off-by: inge4pres <[email protected]>
With last commit i separated the sending_queue and batching sections completely, after learning what sending_queue is via the OTel docs. @carsonip @lahsivjar please let me know if a corresponding change should be in the upstream docs 🙏🏼 |
@theletterf please kick in with the tabs magic if you think the applies_to are too clumsy |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm refraining from nitpicking too much. With this PR it should be a net improvement over the existing docs.
docs/reference/edot-collector/components/elasticsearchexporter.md
Outdated
Show resolved
Hide resolved
docs/reference/edot-collector/components/elasticsearchexporter.md
Outdated
Show resolved
Hide resolved
docs/reference/edot-collector/components/elasticsearchexporter.md
Outdated
Show resolved
Hide resolved
docs/reference/edot-collector/components/elasticsearchexporter.md
Outdated
Show resolved
Hide resolved
@theletterf is it the best practice to backport doc changes? I thought docsv3 are only published from main. |
Co-authored-by: Carson Ip <[email protected]>
@carsonip That's right, but we recently had some issues with the release notes process if docs weren't synced. +CC @colleenmcginnis do you know if we need to continue backporting docs changes for the time being? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SGTM (other than the comment by Carson on sending_queue::batch::enabled
! For future/next release, we have enabled sending queue by default in ES exporter from v0.138.0.
Oh so when we get an upgrade in Elastic Agent for the component, we have to update the docs again?!? |
Co-authored-by: Carson Ip <[email protected]>
Yes 😢
I agree 💯 . We could still do this and open PRs to upstream docs if they are not sufficient. |
Clarified configuration options for sending queue and batcher settings in the Elasticsearch exporter documentation.
Closes #10584
What does this PR do?
Why is it important?
We are not specifying correctly how batching should be configured in each stack release of EDOT.
Checklist
./changelog/fragments
using the changelog toolDisruptive User Impact
How to test this PR locally
Related issues
elasticsearchexporter
batcher config vs sending_queue.batch #10584Questions to ask yourself